Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Step2] 로또(자동) #724

Open
wants to merge 6 commits into
base: kyudong3
Choose a base branch
from
Open

[Step2] 로또(자동) #724

wants to merge 6 commits into from

Conversation

Kyudong3
Copy link

step2 로또(자동) 리뷰 잘 부탁드립니다! 🙏🏼

* 로또 1장의 가격은 1000원이다
* Lotto -> Lottery로 이름 변경
* 구매한 금액에 해당하는 로또 갯수 반환
* 구매 금액을 입력하면 구입 금액에 해당하는 로또를 발급해야 한다
Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

klintFormat을 한번 수행한 다음 커밋을 남겨주세요.

image

그리고 조금 더 개선해보면 좋을 것 같은 부분에 코드리뷰를 남겨두었습니다!

개선 후 다시 리뷰요청 부탁드리겠습니다.

Comment on lines +12 to +14
return readln().split(",").map {
it.trim().toInt()
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드를 연속해서 호출하는 경우에는 행 구분을 해주세요!

https://kotlinlang.org/docs/coding-conventions.html#wrap-chained-calls

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kotlin convention을 다시 한 번 제대로 읽어봐야겠어요 감사합니다! 🙏🏼

Comment on lines +7 to +12
val lotteryTickets = mutableListOf<Lottery>()
repeat(count) {
lotteryTickets.add(
Lottery((MIN_LOTTERY_NUMBER..MAX_LOTTERY_NUMBER).shuffled().take(6).sorted())
)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

가변적인 MutableList를 사용하는 것 보다 불변인 List로 생성하는 것이 더 안정적일 것 같습니다~!

아래와 같이 개선해보면 좋을것 같습니다.

Suggested change
val lotteryTickets = mutableListOf<Lottery>()
repeat(count) {
lotteryTickets.add(
Lottery((MIN_LOTTERY_NUMBER..MAX_LOTTERY_NUMBER).shuffled().take(6).sorted())
)
}
val lotteryTickets = List(count) {
Lottery((MIN_LOTTERY_NUMBER..MAX_LOTTERY_NUMBER).shuffled().take(6).sorted())
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리턴할 때 UserLottery에 List 타입으로 선언해 추가해주었는데, buy 메소드 안에서도 가변적인 리스트보다 불변 리스트를 생성해 리턴하면 더 좋을 것 같네요!! 👍🏼

Comment on lines +5 to +20
fun getWinStatistic(
lotteryTickets: List<Lottery>,
lastWinnerNumbers: List<Int>
): Pair<List<LotteryResult>, Double> {
val winResult = checkLotteryTickets(lotteryTickets, lastWinnerNumbers)

val lotteryResults = mutableListOf<LotteryResult>()
var sum = 0
winResult.forEach { (count, matchCount) ->
val prize = WinnerPrize.getPrize(count).money
val message = "${count}개 일치 (${prize}원) - ${matchCount}개"
lotteryResults.add(LotteryResult(prize, matchCount, message))
sum += (prize * matchCount)
}

return lotteryResults.toList() to sum.toDouble()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List<Lottery>를 가지고 있는 일급컬렉션에게 winningNumbers를 전달하여 위 책임을 위임할 수 있을것 같습니다.

그러면 전체적인 코드가 조금 더 간결해 질 수 있을것 같습니다.

@@ -0,0 +1,18 @@
package lotto

enum class WinnerPrize(val money: Int) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1등에 여러번 당첨되면 Int로는 감당하기 힘들어 질 것 같네요 😄

Comment on lines +12 to +15
3 -> LAST
4 -> THIRD
5 -> SECOND
else -> FIRST
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WinnerPrizecount에 대한 정보도 가지고 있을 수 있지 않을까요?

@@ -0,0 +1,20 @@
package lotto
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

패키지를 구조를 변경하여 domain 과 view 를 분리해주세요.

): List<Pair<Int, Int>> =
lotteryTickets.map { lottery ->
lottery.numbers.intersect(
lastWinnerNumbers.sorted().toSet()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정렬할 필요가 있을까요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다시 생각해보니 sort할 필요는 없는 것 같네요..! 🙏🏼

3 -> LAST
4 -> THIRD
5 -> SECOND
else -> FIRST
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIRST는 전부 맞은것으로 판별하면 되지 않을까요?

lottery.numbers.intersect(
lastWinnerNumbers.sorted().toSet()
).count()
}.filter { it >= 3 }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3개가 넘게 일치하는지를 이곳에서 판별하지 않고

WinnerPrize에게 물어보는 형식으로 변경할 수 있을것 같습니다.

private fun checkLotteryTickets(
lotteryTickets: List<Lottery>,
lastWinnerNumbers: List<Int>
): List<Pair<Int, Int>> =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PairIntInt가 쌍으로 나가는것보다 조금더 의미있는 객체로 내보내는것이 코드를 이해하기 더 수월할 것 같습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants